Skip to content

fix: 2nd-pass audit — CodeQL workflow-permissions alert + dead ok/err helpers marketed as feature#32

Merged
heznpc merged 1 commit into
mainfrom
fix/second-pass-audit-2026-05-21
May 21, 2026
Merged

fix: 2nd-pass audit — CodeQL workflow-permissions alert + dead ok/err helpers marketed as feature#32
heznpc merged 1 commit into
mainfrom
fix/second-pass-audit-2026-05-21

Conversation

@heznpc
Copy link
Copy Markdown
Member

@heznpc heznpc commented May 21, 2026

Previously declared done; second-pass caught these.

This PR audits PR #26 (modernization sweep) and PR #27 (simplify pass) — both reported "tests pass, CI green, merged." Adversarial re-read found 2 Critical findings.

[C1] CodeQL analyze (actions) open alert

[C2] ok() and err() Response Helpers — dead code marketed as feature

  • src/my_mcp_server/server.py:41,47 defined ok() / err() helpers.
  • Zero references anywhere in src/ or tests/. Coverage report confirmed server.py:43-44, 49 uncovered.
  • README.md:32 and README.ko.md:30 marketed these as "Response Helpers — ok() and err() for consistent tool responses" in the "What You Get" list — a public feature claim with no backing usage.
  • The bundled greet tool returns plain str — the README-marketed pattern doesn't match the only bundled example.
  • FastMCP's idiom is return value / raise on error; ok/err helpers are a raw-MCP-without-FastMCP relic that actively teach the wrong pattern.
  • Fix: delete ok()/err() + their introductory comment. Replace the Tools-section banner with one line documenting the actual FastMCP idiom. Remove the "Response Helpers" bullet from both READMEs.

Why this matters

A starter is a teaching artifact. False "What You Get" entries propagate the wrong pattern to every clone. Coverage even improved (70.64% → 72.12%) by deleting marketed-but-dead code surface.

Out-of-scope (Major findings — for user decision, separate PR)

Test plan

  • All 7 workflows now have top-level permissions: (column-1 awk scan)
  • ruff check . / ruff format --check . / mypy src/ clean
  • pytest 17/17, coverage 72.12% (above 70 threshold)
  • CI matrix green
  • CodeQL analyze (actions) alert closes after merge

Adversarial second-pass on commits 01ed305 (PR #26) and ec7045d (PR #27),
both reported as "done / tests pass / CI green". Two Critical findings.

[C1] CodeQL `analyze (actions)` open alert
- Rule: `actions/missing-workflow-permissions`
- File: .github/workflows/cd.yml
- Message: "Actions job or workflow does not limit the permissions of the
  GITHUB_TOKEN."
- Detected by the CodeQL `actions` language matrix that PR #26 itself added.
  PR #26 introduced top-level permissions on most workflows but missed
  cd.yml and setup.yml. The 5 required CI checks didn't include CodeQL,
  so the alert landed silently after merge.
- Fix: add `permissions: contents: read` at workflow top level in cd.yml
  and setup.yml. Jobs continue to opt into the writes they need
  (contents:write / id-token:write / attestations:write for publish;
  issues:write for setup).

[C2] `ok()` and `err()` Response Helpers — dead code marketed as feature
- server.py:41 and server.py:47 defined ok() / err() helpers.
- Zero references anywhere in src/ or tests/ (`grep -rn "\\bok(\\|\\berr("` =
  only the definitions + the introductory comment).
- Coverage report confirmed: server.py:43-44, 49 uncovered.
- README.md:32 and README.ko.md:30 marketed these as "Response Helpers —
  ok() and err() for consistent tool responses" in the "What You Get"
  list — i.e. a public feature claim with no backing code usage.
- The bundled `greet` tool returns plain `str` — the README's marketed
  pattern doesn't match the bundled example.
- FastMCP's idiom is `return value` / `raise on error`; ok/err helpers
  are a raw-MCP-without-FastMCP relic that actively teach the wrong
  pattern.
- Fix: delete ok() / err() / their introductory comment. Replace the
  Tools-section banner comment with one line documenting the actual
  FastMCP idiom. Remove the "Response Helpers" bullet from README.md
  and README.ko.md.

Why this matters beyond cosmetics: a starter is a teaching artifact.
False "What You Get" entries silently propagate the wrong pattern to
every clone. The dead helpers were also reachable code surface that
CodeQL has to scan but no test exercises.

Verification
- All 7 workflows now have top-level `permissions:` (verified via
  `awk` scan of column-1 `permissions:` line).
- ruff / ruff format / mypy strict / pytest 17/17 all clean.
- Coverage 72.12% (improved from 70.64% — uncovered dead code removed).

Out-of-scope for this PR (will be raised as Major findings for user
decision):
- tests/test_tools.py has no `test_greet_registered_on_server` despite
  test_server_info.py and test_code_review.py both having that pattern.
- server_info.py wheel-install fallback path (lines 41-42, 52-56) is 0%
  covered; test only exercises the dev/source-tree read-pyproject path.
- src/my_mcp_server/tools/greet.py defines a `register()` that's never
  called by server.py — fully dead modular example.
- SECURITY.md "branch protection on main" claim is starter-side only;
  clones inherit nothing.
- 4 Dependabot PRs (#28, #29, #30, #31) opened today — #28 is
  attest-build-provenance v3.2.0 → v4.1.0, making our 6-hour-old SHA
  pin already one major behind.
@heznpc heznpc enabled auto-merge (squash) May 21, 2026 12:55
@heznpc heznpc merged commit 98b7892 into main May 21, 2026
8 checks passed
@heznpc heznpc deleted the fix/second-pass-audit-2026-05-21 branch May 21, 2026 12:56
heznpc added a commit that referenced this pull request May 21, 2026
…verage, dead modular example, clone-vs-starter boundary (#33)

Resolves all 4 Major findings from the second-pass audit on PR #32.

[M1] tests/test_tools.py: add `test_greet_registered_on_server`
- Parallels test_server_info.py and test_code_review.py, both of which had
  an equivalent registration assertion already.
- Calls `mcp.list_tools()` and verifies the greet tool is wired in AND that
  the generated JSON Schema actually reflects the Annotated[..., Field(
  min_length, max_length)] constraints. Without this, FastMCP's schema
  generation was an unverified link in the protocol contract.

[M2] tests/test_server_info.py: cover wheel-install fallback path
- The existing tests all walk the source tree and find pyproject.toml.
  That's the dev/editable-install path. When a user does `pip install
  my-mcp-server` from a wheel, pyproject.toml is NOT shipped — _read_pyproject
  returns None and _server_metadata must fall back to
  importlib.metadata.version(). That branch (server_info.py:51-56) was
  previously 0% covered.
- Three new tests via monkeypatch:
  · `_read_pyproject` returns None → version comes from importlib.metadata.
  · `_read_pyproject` returns None AND importlib.metadata raises
    PackageNotFoundError → returns "0.0.0" fallback.
  · pyproject.toml exists but has no [project] table → covers the
    server_info.py:42 exit.
- server_info.py coverage: 79% → 96%.

[M3] src/my_mcp_server/tools/greet.py: delete dead modular example
- The file defined `greet_formal` + a `register()` function, but server.py
  never called `from my_mcp_server.tools.greet import register` to wire it
  in. 100% dead code with 0% test coverage.
- Updated server.py:71-74 comment to describe the modular pattern in
  prose without referencing the deleted file.

[M4] SECURITY.md: distinguish code-level vs runtime GitHub settings
- Previous text "Repo-side toggles enabled — Secret scanning + push
  protection + Dependabot security updates + branch protection on main"
  was misleading for clones: GitHub does NOT copy these settings when a
  template is cloned into a new repo. Clones inherited the false belief
  that "I'm safe because the starter is."
- Added a new section "What clones inherit vs. what they don't" with the
  exact `gh api` invocations clones can run to replicate the toggles on
  their own fork.

Coverage moved 70.64% → 84.69% (also benefits from M3 deletion of the
0%-covered dead module). 21 tests pass (was 17).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant